-
-
Notifications
You must be signed in to change notification settings - Fork 474
Adapt to chacha20 #1642
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adapt to chacha20 #1642
Conversation
I opened #1643. This PR is useful as a draft but won't be merged in its current form (likely we'll want the MSRV/edition bump first as its own PR). I'm not certain on the timeline yet; the main blocker is the |
0fbb567
to
e41e428
Compare
@dhardy I've rebased and the new tests failed as expected (the default ChaCha12 in chacha20 is the IETF variant). I tried to change to the "Legacy" variant, but chacha20 only exports ChaCha20Legacy, not the 12 round variant that we will need here. The ChaCha20Legacy type is just an alias, so I tried to use the underlying type directly to make a 12 round variant ( @tarcieri It seems that I might stuck here until someone either exports |
It's an oversight, we should export it. Could you create a PR? It also may be worth to add aliases for the legacy variants as well. |
@hpenne I'm confused, why do you need a |
@tarcieri When I add the "legacy" feature to the chacha20 crate dependency in the rand cargo.toml, the chacha20 crate fails to build:
Seems to work fine when i build run tests in chacha20 itself. Odd. |
@hpenne all of the RNG types now have a 64-bit counter as of the latest prerelease. Also looks like you found a bug. Perhaps it was being hidden by feature unification? Strange. |
@hpenne oh, you're using an out-of-date version! Please upgrade to v0.10.0-rc.1 |
…sions that are no longer needed.
@tarcieri That was embarrassing. Works much better with the correct version. All tests pass now. The only strangeness that I am left with is that if I do not enable the "legacy" feature (the only enabled feature is "rng"), chacha20 fails to build with:
I'm not able to reproduce this when I build chacha20 locally from master, so perhaps I've done something wrong or you have already fixed this on master. I'll look closer tomorrow. |
@hpenne aah that was RustCrypto/stream-ciphers#454 which has been fixed I can cut an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I did not think to appraise you of the chacha20
changes. I think we have all the pieces to make a rand pre-release now.
examples/rayon-monte-carlo.rs
Outdated
rng.set_stream(i); | ||
rng.set_stream(u64::from(i)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i
should be u64
already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I used to need to convert to u128 with the old chacha20. I had to change u64 when I rebased but failed to notice that I could simply remove the conversion. Will fix.
Cargo.toml
Outdated
rand_chacha = { path = "rand_chacha", version = "0.9.0", default-features = false, optional = true } | ||
chacha20 = { version = "=0.10.0-rc.1", default-features = false, features = ["rng", "legacy"], optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can depend on the "legacy" feature for now, but add a TODO
comment to remove it on the next version.
CHANGELOG.md
Outdated
@@ -10,6 +10,7 @@ You may also find the [Upgrade Guide](https://rust-random.github.io/book/update. | |||
|
|||
## [0.10.0 — Unreleased] | |||
### Changes | |||
- The dependency on `rand_chacha` has been replaced with a dependency on `chacha20`. This changes the implementation behind `StdRng`, but the output remains the same. There may be some API breakage when using the ChaCha-types directly as these are now the ones in `chacha20` instead of `rand_chacha`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Include the PR # please.
…:from in rayon-monte-carlo.rs. Added a ToDo in Cargo.toml to remove the "legacy" feature of chacha20.
Cargo.toml
Outdated
# ToDo: Remove the "legacy" feature from chacha20 when this is not longer necessary | ||
chacha20 = { version = "=0.10.0-rc.1", default-features = false, features = ["rng", "legacy"], optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've released an rc.2
with the feature-related bugfix:
# ToDo: Remove the "legacy" feature from chacha20 when this is not longer necessary | |
chacha20 = { version = "=0.10.0-rc.1", default-features = false, features = ["rng", "legacy"], optional = true } | |
# ToDo: Remove the "legacy" feature from chacha20 when this is not longer necessary | |
chacha20 = { version = "=0.10.0-rc.2", default-features = false, features = ["rng"], optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I removed the ToDo-comment as well, now that the "legacy" feature is no longer necessary.
CHANGELOG.md
entrySummary
Replaced the dependency on
rand_chacha
with one onchacha20
. Added some tests tostd.rs
to ensure that the output ofStdRng
did not change.Fixes #934.
Motivation
Reduces total code size and the total amount of unsafe code.
Details
Changes to config.toml and some replacement of
rand_chacha::
withchacha20::
.Added three new unit tests to std.rs. These were based on tests of IETF test vectors from
rand_chacha
, but the actualexpected
values had to be replaced, as the IETF test vectors are for ChaCha20 whilerand
uses ChaCha12. Theexpected
values were generated by usingrand_chacha
(beforechacha20
was used) to verify that the algorithm change did not affect the output.